Add snapshot deployment example #31

okartau · 2019-03-29T13:04:41Z

Migrated the snapshot example from docs/example to here,
with minor updates and cleanup.
Corrected misspelled snpshot in deploy file name.

k8s-ci-robot · 2019-03-29T13:04:54Z

Hi @okartau. Thanks for your PR.

I'm waiting for a kubernetes-csi or kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

pohly · 2019-03-29T15:22:26Z

/ok-to-test

pohly · 2019-03-29T15:23:21Z

README.md

@@ -14,7 +14,7 @@ The easiest way to test the Hostpath driver is to run `deploy/deploy-hostpath.sh
 $ sh deploy/deploy-hostpath.sh
 ```

-You should see an output similar to the following printed on the terminal showing the application of rbac rules and the result of deploying the hostpath driver, external privisioner and external attacher components:
+You should see an output similar to the following printed on the terminal showing the application of rbac rules and the result of deploying the hostpath driver, external privisioner, external attacher and snapshotter components:


Chance for a drive-by spelling fix:
s/privisioner/provisioner/

yes, those 4 spelling errors sting my eye every time, but there is #2 to fix those and I did not want to step on toes of another ongoing change. I can sure fix in current PR if so desired.

Please include the fix. Otherwise we just end up with merge conflicts.

yes, makes sense, as this PR makes massive changes anyway. And I proposed few of #2 fixes there, so I hope its not that intrusive. Will change and re-push.
BTW, while we discuss spell etc minor issues, I want to propose: lets get rid of explicit shell call in deploy script start cmd. As the script has x-bits set, it can be started without specifying shell explicitly, and I dont see extra value from explicit sh, is there?

I included all fixes of #2 now, plus removed explicit sh in deploy cmd, force-pushed

lets get rid of explicit shell call in deploy script start cmd

You mean #!/usr/bin/env bash?

As the script has x-bits set, it can be started without specifying shell explicitly,

Leave out the shebang line entirely? I doubt that this is POSIX-compliant. For example, does it work because the shell itself then reads the input or is it the Linux kernel which falls back to running in a shell? A quick test (create executable file with sleep 120, invoke in script in dash and bash, look at /proc/*/maps of the script) shows that it is a feature of the shell: the script then always runs in the same shell as the one it was invoked in - if the shell supports that.

and I dont see extra value from explicit sh, is there?

Last but not least, it does matter. The script uses bash features (set -o pipefail) and therefore bash has to be set explicitly.

plus removed explicit sh in deploy cmd

I'm not seeing that in the latest changes. Are you sure you force-pushed?

I was not clear enough in my comments. I did not mean deploy script contents, but how it's activation is written in README (where user typically copies commands from).
Was: sh deploy/deploy-hostpath.sh
I changed to: deploy/deploy-hostpath.sh

Oh, absolutely. Invoking it in sh (which could be dash) is just plain wrong.

pohly · 2019-03-29T15:24:09Z

README.md

@@ -51,7 +52,7 @@ The [livenessprobe side-container](https://github.com/kubernetes-csi/livenesspro

 ## Run example application and validate

-Next, validate the deployment.  First, ensure all expected pods are running properly including the external attacher, provisioner, and the actual hostpath driver plugin:
+Next, validate the deployment.  First, ensure all expected pods are running properly including the external attacher, provisioner, snapshotter and the actual hostpath driver plugin:


The following output probably also needs to be updated?

no, it was updated with one of previous PRs (added snapshotter pod). In current PR I noticed the list in text lacks the snapshotter

pohly · 2019-03-29T15:24:48Z

README.md

@@ -188,6 +189,141 @@ Events:      <none>
 ```


+## Snapshot support
+
+Since volume snapshot is an alpha feature in Kubernetes v1.12 and v1.13, you need to enable feature gate called `VolumeSnapshotDataSource` in the Kubernetes API server binary.


@xing-yang it's also alpha in 1.14, right?

It also needs to be enabled in kube-controller-manager.

(and yes, alpha in 1.14)

I will add v1.14

What is good wording to mention VolumeSnapshotDataSource then?
I verified deployment in 4xVM cluster that is part of pmem-csi. There I got VolumeSnapshotDataSource added to cluster setup using config file given to "kubeadm init", so in practice I think one does not "enable gate in binary" as current sentence is, but adds gating flag in some config file or init step.

I think just saying "you need to enable the VolumeSnapshotSource feature gate in Kubernetes" is good enough. The actual mechanism to enable them depends on what deployment method is used.

Also regarding the alpha versions, maybe just saying "starting in Kubernetes v1.12" is good enough. Then we don't have to keep on updating the the example every Kubernetes release.

pushed new state with changes discussed here in recent comments

pohly

/lgtm

Migrated the snapshot example from docs/example to here, with minor updates and cleanup. Corrected misspelled snpshot in deploy file name.

msau42

/lgtm
/approve

Just one minor wording nit that can be done in a followup

msau42 · 2019-04-01T20:20:35Z

README.md

@@ -188,6 +189,143 @@ Events:      <none>
 ```


+## Snapshot support
+
+Since volume snapshot is an alpha feature starting in Kubernetes v1.12, you need to enable feature gate called `VolumeSnapshotDataSource` in the Kubernetes.


nit: reword as

"Volume snapshot is an alpha feature introduced in Kubernetes v1.12. The Kubernetes feature gate VolumeSnapshotDataSource must be enabled."

k8s-ci-robot · 2019-04-01T20:21:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: msau42, okartau

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [msau42]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

pohly · 2019-04-01T20:28:50Z

/hold

The script has a race condition: the snapshot class gets created without waiting for the snapshotter to create the corresponding CRD.

I suggest we merge this as part of PR #29 with a fix.

pohly · 2019-04-01T20:29:36Z

Oops, too late. Let's follow up with the fix in PR #29 then.

k8s-ci-robot requested review from jsafrane and msau42 March 29, 2019 13:04

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 29, 2019

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 29, 2019

okartau mentioned this pull request Mar 29, 2019

example.md: reduce the contents which are present in hostpath deployment example kubernetes-csi/docs#125

Merged

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 29, 2019

pohly requested changes Mar 29, 2019

View reviewed changes

okartau force-pushed the complete-snapshot-deployment branch from cfd6397 to 13e1067 Compare March 29, 2019 21:30

okartau mentioned this pull request Mar 29, 2019

Fix minor typos #2

Closed

okartau force-pushed the complete-snapshot-deployment branch from 13e1067 to 74db762 Compare March 29, 2019 21:42

k8s-ci-robot assigned pohly Apr 1, 2019

pohly approved these changes Apr 1, 2019

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 1, 2019

Add snapshot deployment example

3180e18

Migrated the snapshot example from docs/example to here, with minor updates and cleanup. Corrected misspelled snpshot in deploy file name.

okartau force-pushed the complete-snapshot-deployment branch from 74db762 to 3180e18 Compare April 1, 2019 19:08

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 1, 2019

msau42 reviewed Apr 1, 2019

View reviewed changes

k8s-ci-robot assigned msau42 Apr 1, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 1, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 1, 2019

k8s-ci-robot merged commit c07eae1 into kubernetes-csi:master Apr 1, 2019

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 1, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add snapshot deployment example #31

Add snapshot deployment example #31

okartau commented Mar 29, 2019

k8s-ci-robot commented Mar 29, 2019

pohly commented Mar 29, 2019

pohly Mar 29, 2019

okartau Mar 29, 2019

pohly Mar 29, 2019

okartau Mar 29, 2019

okartau Mar 29, 2019

pohly Apr 1, 2019

pohly Apr 1, 2019

okartau Apr 1, 2019

pohly Apr 1, 2019

pohly Mar 29, 2019

okartau Mar 29, 2019

pohly Mar 29, 2019

msau42 Apr 1, 2019

msau42 Apr 1, 2019

okartau Apr 1, 2019

okartau Apr 1, 2019

msau42 Apr 1, 2019

msau42 Apr 1, 2019

okartau Apr 1, 2019

pohly left a comment

msau42 left a comment

msau42 Apr 1, 2019

k8s-ci-robot commented Apr 1, 2019

pohly commented Apr 1, 2019

pohly commented Apr 1, 2019

Add snapshot deployment example #31

Add snapshot deployment example #31

Conversation

okartau commented Mar 29, 2019

k8s-ci-robot commented Mar 29, 2019

pohly commented Mar 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pohly left a comment

Choose a reason for hiding this comment

msau42 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 1, 2019

pohly commented Apr 1, 2019

pohly commented Apr 1, 2019